8452 multiple coll mode #8473
8452 multiple coll mode #8473
Conversation
…cab., and changing the ddi export accordingly. #8452
There was a problem hiding this comment.
It's an interesting backward compatibility question re: whether to check for single values (per the reviewer note) - I think that makes sense - just from the perspective of not making the new install break if you test before updating the block.
(If I understand correctly, the only indication of multiple is in the field type (other than that multiple could have several datasetfieldvalue entries associated with one datasetfield) so there's no problem with existing/legacy entries (since we're going in the single-multiple direction)). So there's no need to have the if single logic for this reason).
I guess I'm OK with either with a slight preference to add the check for the single valued type.
Otherwise - just the one issue where I think the block edit now has it as CVV without any values.
| writeI18NElement(xmlw, "collMode", version, DatasetFieldConstant.collectionMode, lang); | ||
| FieldDTO collModeFieldDTO = dto2FieldDTO(version, DatasetFieldConstant.collectionMode, "socialscience"); | ||
| if (collModeFieldDTO != null) { | ||
| writeI18NElementList(xmlw, "collMode", collModeFieldDTO.getMultipleVocab(), DatasetFieldConstant.collectionMode, collModeFieldDTO.getTypeClass(), "socialscience", lang); |
There was a problem hiding this comment.
FWIW: This works because getMultipleVocab and getMultiplePrimitive do the same thing (w.r.t. the block above isn't using CVV whereas SciencesPO's block is).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java
Outdated
Show resolved
Hide resolved
| "value": "CollectionMode" | ||
| "value": [ | ||
| "CollectionMode" | ||
| ] |
There was a problem hiding this comment.
Does this mean that the native JSON format is changing? If so we should indicate this under "Backward Incompatibilities" in the release notes.
There was a problem hiding this comment.
I believe the answer is "yes". (I'm not 100% sure, tbh. the above change in the .json file was needed for the tests to pass. There is a small chance that a real json import would still work; and the single primitive value would be accepted... I should've tested that).
OK, I'll look for an example of an advertised backward incompatibility in the past release notes.
There was a problem hiding this comment.
Thanks. If it helps, the 5.10 release notes already have a "Backwards Incompatibilities" section.
There was a problem hiding this comment.
You're right, thanks!
There was a problem hiding this comment.
It is an incompatibility, yes.
|
@kcondon I have added some language to the main release notes directly, about the schema update that's needed in order to index any such multiple fields. Since we are recommending a from-scratch install of the latest stable Solr version as part of the release, the installations that follow that recommendation will end up with the latest schema in the process. But I did spell it out for any holdouts too. |
|
Note this requires the schema to be updated. |
1 similar comment
|
Note this requires the schema to be updated. |
What this PR does / why we need it:
Makes the field collectionMode multiple.
There's one installation where they made these changes in the custom copy of the block they are using. It seems like a sensible modification. But it is now resulting in failed DDI exports for them on datasets that actually have collectionMode fields populated; since the code in ddiExportUtil was hard-coded to a single primitive value. (more details in the linked issue)
Which issue(s) this PR closes:
Closes #8452
Special notes for your reviewer:
My main question is, was I supposed to leave backward compatibility code in there, for an installation that upgrades to 5.10, but forgets to re-import the updated social_science.tsv?
So, something along the lines of
in its current form it just assumes that it's a multiple field.
Suggestions on how to test this:
Please update the metadata block (use the tsv from my branch - https://github.com/IQSS/dataverse/blob/8452-multiple-collMode/scripts/api/data/metadatablocks/social_science.tsv; the url in the release note isn't going to work until we release...); create a dataset with multiple collMode values; export as DDI, confirm that the multiple values are in the resulting XML.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: